Skip to content

Conversation

@Arlodotexe
Copy link
Member

@Arlodotexe Arlodotexe commented Feb 24, 2023

This PR is a hotfix for all components that may use a Storyboard. Our CI treats all warnings as errors in release mode, so this has been suppressed until Uno can fix it.

From here:

Some objects are marked as disposable for internal constraints, but it's not something we can change right away as it may be a breaking change (we'll take note to add it as such for our next major). Having to add IDisposable here is likely a result of some analyzer that can be disabled for Uno targets specifically.

@Arlodotexe Arlodotexe added bug 🐛 Something isn't working Uno Issues related to Uno Platform dev loop ➰ For issues that impact the core dev-loop of building experiments labels Feb 24, 2023
@Arlodotexe Arlodotexe marked this pull request as draft February 24, 2023 19:41
@Arlodotexe Arlodotexe marked this pull request as ready for review February 24, 2023 19:52
@michael-hawker
Copy link
Member

@Arlodotexe isn't their an open Uno issue on this too, can we find that and link the url to the comment in the config too?

I didn't see it linked on our board, so if there is one we should add it there for tracking too: Windows Community Toolkit (view)

@Arlodotexe
Copy link
Member Author

@Arlodotexe isn't their an open Uno issue on this too, can we find that and link the url to the comment in the config too?

I didn't see it linked on our board, so if there is one we should add it there for tracking too: Windows Community Toolkit (view)

Going back to the original comments from @jeromelaban and looking through open Uno issues, I wasn't able to find an existing issue for this.

@michael-hawker
Copy link
Member

@Arlodotexe, want to open one so we can link it to all the places and track then? I know @jeromelaban mentioned it'd take a version to fix or something as it'd be a breaking change. Not sure if he meant a 4.8 or a 5.0 sort of thing though.

@jeromelaban
Copy link
Collaborator

jeromelaban commented Feb 27, 2023

@michael-hawker IDisposable on types exposed on iOS, Android and Catalyst will continue (it's coming from .NET bindings to native UIKit/Java types) and this not something we'll change any time soon. This is a characteristic of Uno's Visual Tree inheriting directly from native types for performance considerations.

@Arlodotexe
Copy link
Member Author

Arlodotexe commented Feb 27, 2023

@jeromelaban Wouldn't it be better for Uno to suppress this warning from the nuget package, if the end user doesn't need to dispose of these? Is that something that's been investigated before?

@jeromelaban
Copy link
Collaborator

@Arlodotexe It could be something possible, though it would be a general exclusion of that warning regardless of the location. Unless I'm mistaken, this happens at the reference site and I don't think analyzers allow this kind of flexibility.

@michael-hawker
Copy link
Member

Random idea I had based on this discussion and the Marquee PR. We could use the Uno props file to only ignore the warning for Uno builds, like this:

<PropertyGroup Condition="'$(IsWpfHead)' == 'true'">
<!-- Ignorable issue from SkiaSharp package, see: https://github.com/CommunityToolkit/Labs-Windows/pull/119#issuecomment-1125373091 -->
<NoWarn>NU1701</NoWarn>
</PropertyGroup>

If there's still a disposable issue it should be flagged from UWP/WinAppSDK bits still, but at least not flag it for other places based on this type of issue, eh?

@Arlodotexe
Copy link
Member Author

Random idea I had based on this discussion and the Marquee PR. We could use the Uno props file to only ignore the warning for Uno builds, like this:

<PropertyGroup Condition="'$(IsWpfHead)' == 'true'">
<!-- Ignorable issue from SkiaSharp package, see: https://github.com/CommunityToolkit/Labs-Windows/pull/119#issuecomment-1125373091 -->
<NoWarn>NU1701</NoWarn>
</PropertyGroup>

If there's still a disposable issue it should be flagged from UWP/WinAppSDK bits still, but at least not flag it for other places based on this type of issue, eh?

I like this idea. If we globally-suppress only on Uno, this error will show on the UWP / WinAppSDK heads instead, and users won't have to manually suppress anything for their control like MarqueeText did.

The only problem with this I can foresee is that a component which exclusively targets Uno won't see this error and may end up with a memory leak. All of our components target UWP or WinAppSDK, so this isn't a problem for now.

We'll move forward with this as a workaround.

This was referenced Mar 8, 2023
@Arlodotexe Arlodotexe marked this pull request as draft March 8, 2023 23:33
@Arlodotexe Arlodotexe closed this May 26, 2023
@Arlodotexe Arlodotexe deleted the fix/workaround/uno/storyboard-idisposable-in-release branch May 26, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐛 Something isn't working dev loop ➰ For issues that impact the core dev-loop of building experiments Uno Issues related to Uno Platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants